GH-45167: [C++] Implement Compute Equals for List Types #45272
Conversation
|
|
github-actions
bot
added
awaiting committer review
WillAyd
force-pushed
the
implement-list-compare
branch
from
573867e to
649e3b0
Compare
WillAyd
force-pushed
the
implement-list-compare
branch
from
649e3b0 to
68bb513
Compare
| explicit ArrayIterator(const ArraySpan& arr) : arr(arr), position(0) {} | ||
|
|
||
| T operator()() { | ||
| const auto array_ptr = arr.ToArray(); |
There was a problem hiding this comment.
The alternative to calling ToArray with the cast would be to implement something like value_slice on the ArraySpan directly, although I'm not sure if the ArraySpan is supposed to return anything but pointers to primitives (as is currently implemeted)
There was a problem hiding this comment.
This is going to be slow, so we probably want to avoid this IMHO.
You may want to run a crude benchmark from Python to check this.
|
@pitrou @jorisvandenbossche would either of you be able to take a look here? |
| @@ -445,6 +445,14 @@ std::shared_ptr<ScalarFunction> MakeCompareFunction(std::string name, FunctionDo | |||
| DCHECK_OK(func->AddKernel({ty, ty}, boolean(), std::move(exec))); | |||
| } | |||
|
|
|||
| if constexpr (std::is_same_v<Op, Equal> || std::is_same_v<Op, NotEqual>) { | |||
| for (const auto id : {Type::LIST, Type::LARGE_LIST}) { | |||
| auto exec = GenerateList<applicator::ScalarBinaryEqualTypes, BooleanType, Op>(id); | |||
There was a problem hiding this comment.
Another approach with perhaps a better performance potential would be to leverage the existing RangeDataEqualsImpl in arrow/compare.cc.
There was a problem hiding this comment.
Thanks for the heads up - I will give that a look. So I see all of the functions right now in the compare module are registered via RegisterScalarComparison. With what you are suggesting, I'm guessing I should be creating a new registry function along with that like RegisterRangeComparison right?
There was a problem hiding this comment.
Also I'm guessing the RangeDataEqualsImpl is supposed to work when comparing two arrays, but not when comparing an array with a scalar
FWIW though I did benchmark the current implementation and it was definitely slow. Seemed about 1000x slower than an equivalent comparison using primitive types
There was a problem hiding this comment.
With what you are suggesting, I'm guessing I should be creating a new registry function along with that like
RegisterRangeComparisonright?
I think we can avoid that by directly calling into RangeDataEqualsImpl.
Also I'm guessing the RangeDataEqualsImpl is supposed to work when comparing two arrays, but not when comparing an array with a scalar
A list scalar's value is actually an array, so that should not necessarily be a problem.
WillAyd
force-pushed
the
implement-list-compare
branch
from
68bb513 to
3b055a8
Compare
|
I currently have no time to review this in depth, but API-wise one remark: right now (for primitive arrays), nulls propagate in an operation like |
|
The current (rather slow) implementation just does an elementwise compare, dispatching to the logical list scalar type. Therefore, since: >>> l1 = pa.scalar([], type=pa.list_(pa.int32()))
>>> l2 = pa.scalar([], type=pa.list_(pa.int32()))
>>> l1 == l2
TrueWrapping that in an array does not change the behavior: >>> arr1 = pa.array([l1])
>>> arr2 = pa.array([l2])
>>> pc.equal(arr1, arr2)
<pyarrow.lib.BooleanArray object at 0x71e8e9232560>
[
true
] |
Rationale for this change
While equality exists for ListScalars, it is not available through the compute module. This makes that now possible.
What changes are included in this PR?
I have added equals and not_equals support to the compute module for list types
Are these changes tested?
Yes - see added changes
Are there any user-facing changes?
Yes - the new feature to allow list comparison through the compute module